Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix color picker saturation bug #23272

Merged
merged 2 commits into from
Jun 22, 2020
Merged

Fix color picker saturation bug #23272

merged 2 commits into from
Jun 22, 2020

Conversation

jasmussen
Copy link
Contributor

Fixes #19610. Alternative to #19693.

A bug exists where if you have a fully saturated color, the color dropper which is supposed to sit at the far right edge, causes the parent container to shift and mess up the layout.

This PR fixes that, by allowing the eye dropper to overflow:

color

It also adds a focus style.

@jasmussen jasmussen added the [Type] Bug An existing feature does not function as intended label Jun 18, 2020
@jasmussen jasmussen self-assigned this Jun 18, 2020
@github-actions
Copy link

github-actions bot commented Jun 18, 2020

Size Change: +23 B (0%)

Total Size: 1.12 MB

Filename Size Change
build/components/style-rtl.css 15.9 kB +12 B (0%)
build/components/style.css 15.8 kB +11 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.4 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 7.26 kB 0 B
build/block-directory/style-rtl.css 937 B 0 B
build/block-directory/style.css 937 B 0 B
build/block-editor/index.js 106 kB 0 B
build/block-editor/style-rtl.css 10.7 kB 0 B
build/block-editor/style.css 10.7 kB 0 B
build/block-library/editor-rtl.css 7.86 kB 0 B
build/block-library/editor.css 7.87 kB 0 B
build/block-library/index.js 129 kB 0 B
build/block-library/style-rtl.css 8 kB 0 B
build/block-library/style.css 8.01 kB 0 B
build/block-library/theme-rtl.css 730 B 0 B
build/block-library/theme.css 732 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 195 kB 0 B
build/compose/index.js 9.6 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.44 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.17 kB 0 B
build/edit-navigation/index.js 8.26 kB 0 B
build/edit-navigation/style-rtl.css 1.02 kB 0 B
build/edit-navigation/style.css 1.02 kB 0 B
build/edit-post/index.js 303 kB 0 B
build/edit-post/style-rtl.css 5.5 kB 0 B
build/edit-post/style.css 5.5 kB 0 B
build/edit-site/index.js 16.6 kB 0 B
build/edit-site/style-rtl.css 3.03 kB 0 B
build/edit-site/style.css 3.03 kB 0 B
build/edit-widgets/index.js 9.33 kB 0 B
build/edit-widgets/style-rtl.css 2.43 kB 0 B
build/edit-widgets/style.css 2.43 kB 0 B
build/editor/editor-styles-rtl.css 468 B 0 B
build/editor/editor-styles.css 469 B 0 B
build/editor/index.js 44.8 kB 0 B
build/editor/style-rtl.css 3.8 kB 0 B
build/editor/style.css 3.8 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.72 kB 0 B
build/format-library/style-rtl.css 542 B 0 B
build/format-library/style.css 543 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 446 B 0 B
build/list-reusable-blocks/style.css 447 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 662 B 0 B
build/nux/style.css 657 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 788 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thank you! It is relieving to see that there is a CSS-only fix to this.

I'm not really able to reproduce the issue as it was back in january, but I'll keep looking.

It looks like the issue changed slightly. That is it is no longer triggered by the right-edge of the picker but the bototm-edge:
bottom-edge-issue

This PR does fix this, and also makes the picker look and behave much better in general with how it is able to overflow.

I also noticed this odd issue with the popover location just now (however, also on master it doesn't seem to be a result of this change):
funky-color-picker

I haven't noticed this before. And I have used the color picker in the last week while helping develop some of the experimental block style support flags. Im not sure whats going here, but I figured it would be worth pointing out.

@@ -27,7 +27,7 @@

.components-color-picker {
width: 100%;
overflow: hidden;
overflow: visible;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im not finding this change necessary to fix the bug. However, the change to visible in .components-color-picker__saturation-color does seem to be what does it!

Did you have any other reasons for changing the rule on this selector as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for the review!

It might be a remnant of me web-inspecting and trying out a few things, let me see if I can remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does seem like I can remove that. So I did. Thanks again.

I rebased in hopes that the checks will now pass. 🤞

@jasmussen jasmussen merged commit 2833411 into master Jun 22, 2020
@jasmussen jasmussen deleted the try/fix-color-picker branch June 22, 2020 06:28
@github-actions github-actions bot added this to the Gutenberg 8.4 milestone Jun 22, 2020
This was referenced Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Color Picker saturation overlay is offset until saturation picker moves
2 participants